Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Stop using CallAllUnordered in peer_set::add_initial_peers" #1734

Merged
merged 2 commits into from
Feb 15, 2021

Conversation

teor2345
Copy link
Contributor

Reverts #1705

This change is safe because:

  • the underlying bug in tower::Buffer has been fixed, see Make sure Zebra uses poll_ready and Buffer reservations correctly #1593
  • the underlying Inbound service is wrapped in a load_shed layer, so it's not possible for it to hang, even if these requests use up one reservation per clone
  • there are 3-4 initial peers by default, and the default buffer size of 20 allows 5+ inbound requests per peer
  • some of those inbound requests will immediately fail because the network isn't ready yet, and that helps avoid hangs

See the buffer bound and load shed code:
https://github.com/ZcashFoundation/zebra/blob/main/zebrad/src/commands/start.rs#L71

@teor2345 teor2345 added A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup P-Medium labels Feb 15, 2021
@teor2345 teor2345 added this to the 2021 Sprint 3 milestone Feb 15, 2021
@teor2345 teor2345 self-assigned this Feb 15, 2021
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR effectively reverts #1705

Correctness comment looks good.

@teor2345 teor2345 merged commit e85441c into main Feb 15, 2021
@teor2345 teor2345 deleted the revert-1705-issue1696 branch February 15, 2021 19:52
@teor2345 teor2345 mentioned this pull request Feb 23, 2021
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants